Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update rules_apple to 2.0.0 #633

Merged

Conversation

mattrobmattrob
Copy link
Collaborator

@mattrobmattrob mattrobmattrob commented Dec 23, 2022

Update rules_apple to https://github.com/bazelbuild/rules_apple/releases/tag/2.0.0:

These parameter changes are likely breaking for folks with a different rules_apple so we'll want to take that into account before merging this PR. This is currently blocked by #632.

# Summary
Only validate the dynamic frameworks (those with `apple_common.AppleDynamicFramework`) when calling the `partials.extension_safe_validation_partial` partial. This mimics the `rules_apple` approach more correctly:

https://github.com/bazelbuild/rules_apple/blob/eb5c03a606b103e9f8b72de954103eb00cf127f7/apple/internal/ios_rules.bzl#L1049-L1053
@mattrobmattrob mattrobmattrob force-pushed the mr/update.rules_apple.to.2.0.0 branch 2 times, most recently from d20e203 to 0bf82ba Compare December 23, 2022 17:05
Update `rules_apple` to https://github.com/bazelbuild/rules_apple/releases/tag/2.0.0:

- Handle bazelbuild/rules_apple@c23720d by adding `cc_info` param to `framework_provider_partial` call.
- Handle bazelbuild/rules_apple@51d6273 by removing `bin_root_path`/`rule_label` param to `debug_symbols_partial` call.
- Handle bazelbuild/rules_apple@a4de222 by removing the `apple_xcframework.framework_type` param.
Copy link
Contributor

@jerrymarino jerrymarino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one @mattrobmattrob 🥳 - LGTM but leaving a minor comment around this test case, the linked rules_apple code would fix a bug for me but if this unblocks it for you I can followup and fix it

@@ -35,7 +35,6 @@ apple_xcframework(
name = "iOSSwiftXCFramework",
bundle_id = "com.google.example",
bundle_name = "c",
framework_type = ["dynamic"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this now automatically detect this? If so - nice 👍 !

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like it was only ever supported as a single value and now it's just not an attribute at all: bazelbuild/rules_apple@a4de222

@@ -1,7 +1,7 @@
load("//rules:framework.bzl", "apple_framework")

apple_framework(
name = "NestedHeaders",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nice thing here - detecting Headers captures and tests an obscure usage and some of bugs in rules_ios like
https://github.com/bazelbuild/rules_apple/pull/1798/files 🙈

Is there any way we can undo this now that you've got it fixed in rules_apple. Also I wonder why objc_import is even dealing with headers now - I think there was a limitation around it that caused it to only pull linkable files like .a or .so

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, since I've gotten it fixed in rules_apple, I'm happy to revert this change and update to an even newer rules_apple commit.

Handle https://github.com/bazelbuild/rules_apple/pull/1552/files or bazelbuild/rules_apple@5d7d31d by self-identifying the `modulemap` file(s) since the forwarding of them in `ObjcProvider` was removed.
@mattrobmattrob
Copy link
Collaborator Author

Asking for another look from you, @jerrymarino. Would be nice to get #632 merged before this one as well.

@mattrobmattrob
Copy link
Collaborator Author

And since this PR/rules_ios doesn't have bazelbuild/rules_apple#1797 yet, I've filed #636 to undo the test dir rename once that commit is pulled in via rules_apple.

@mattrobmattrob mattrobmattrob marked this pull request as ready for review January 9, 2023 23:51
Copy link
Contributor

@jerrymarino jerrymarino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢

@mattrobmattrob mattrobmattrob merged commit 9a705f8 into bazel-ios:master Jan 10, 2023
@mattrobmattrob mattrobmattrob deleted the mr/update.rules_apple.to.2.0.0 branch January 10, 2023 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants